Muscle FiberVelocityInfo and MuscleDynamicsInfo refactor#14
Draft
pepbos wants to merge 13 commits intodraft-merge-targetfrom
Draft
Muscle FiberVelocityInfo and MuscleDynamicsInfo refactor#14pepbos wants to merge 13 commits intodraft-merge-targetfrom
pepbos wants to merge 13 commits intodraft-merge-targetfrom
Conversation
adamkewley
reviewed
Dec 13, 2023
| FiberVelocityInfo& fvi, const SimTK::Real& normTendonForce, | ||
| const SimTK::Real& normTendonForceDerivative) const { | ||
|
|
||
| // Multipliers. |
|
|
||
| double normFiberVelocity = SimTK::NaN; | ||
| if (isTendonDynamicsExplicit && !ignoreTendonCompliance) { | ||
| const auto& normFiberForce = normTendonForce / mli.cosPennationAngle; |
Collaborator
There was a problem hiding this comment.
Reference to temporary?
| muscleTendonVelocity - tendonVelocity; | ||
| fvi.fiberVelocity = | ||
| fvi.fiberVelocityAlongTendon * mli.cosPennationAngle; | ||
| fvi.normFiberVelocity = |
Collaborator
There was a problem hiding this comment.
The original code updates normFiberVelocity, whereas the new version NaNs it?
| fvi.normTendonVelocity = fvi.tendonVelocity / get_tendon_slack_length(); | ||
| const double tendonVelocity = | ||
| muscleTendonVelocity - fiberVelocityAlongTendon; | ||
| const double normTendonVelocity = tendonVelocity / get_tendon_slack_length(); |
Collaborator
There was a problem hiding this comment.
These local variables seem to have no purpose?
| normFiberVelocity = | ||
| calcForceVelocityInverseCurve(fvi.fiberForceVelocityMultiplier); | ||
| fvi.fiberVelocity = fvi.normFiberVelocity * | ||
| fvi.fiberVelocity = normFiberVelocity * |
Collaborator
There was a problem hiding this comment.
These are the only side-effect-full parts of this block scope?
| double fpe = fpeCurve.calcValue(mli.normFiberLength); | ||
|
|
||
| // Get multipliers specific to this muscle. | ||
| const ForceMultipliersCV multipliers = getForceMultipliers(s); |
|
|
||
| void calcFiberVelocityInfo(const SimTK::State& s, | ||
| const MuscleLengthInfo& mli, | ||
| FiberVelocityInfo& fvi) const override; |
Collaborator
There was a problem hiding this comment.
You plan on making this override-able?
| fvi.fiberActiveForceLengthMultiplier = | ||
| get_active_force_length_curve().calcValue(arg); | ||
| fvi.fiberPassiveForceLengthMultiplier = | ||
| SimTK::clamp(0, get_passive_force_length_curve().calcValue(arg), 10); |
Collaborator
There was a problem hiding this comment.
Why is this arbitrarily clamped between 0 and 10?
| //1. Get fiber/tendon kinematic information | ||
|
|
||
| //clamp activation to a legal range | ||
| double a = getActivationModel().clampActivation(getStateVariableValue(s, |
| MuscleDynamicsInfo(): | ||
| activation(SimTK::NaN), | ||
| FiberVelocityInfo(): | ||
| fiberVelocity(SimTK::NaN), |
Collaborator
There was a problem hiding this comment.
Use member initializers instead?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR combines
Muscle::FiberVelocityInfoandMuscle::MuscleDynamicsInfointo a single cached variable, and removes member fields that are cheap to compute.Considering that fetching memory is slower than multiplying a few numbers, the following fields are cheap to compute, and are removed:
FiberVelocityInfo::fiberVelocityAlongTendon,FiberVelocityInfo::normFiberVelocity,FiberVelocityInfo::pennationAngularVelocity,FiberVelocityInfo::normTendonVelocity,MuscleDynamicsInfo::fiberForceAlongTendon,MuscleDynamicsInfo::normFiberForce,MuscleDynamicsInfo::normTendonForce,MuscleDynamicsInfo::fiberStiffnessAlongTendon,MuscleDynamicsInfo::muscleStiffness,MuscleDynamicsInfo::tendonPower,MuscleDynamicsInfo::musclePower,This leaves two fields in
FiberVelocityInfo.When computing the fiber velocity, with the exeption of
Millard2012AccelerationMuscle, all muscles need to compute the forces as well. The forces can thus be seen as a byproduct of computing the velocities,Furthermore, the
FiberVelocityInfoandMuscleDynamicsInfocached variables are invalidated at the same stage (Stage::Velocity).Finally, it turns out that distributing the
Muscleinformation over 3 cached variables is slower than over 2.Since computing the forces is actually part of computing the muscle velocities, I argue for combining
MuscleDynamicsInfowithFiberVelocityInfo.Performance
Measuring the wall clock time for several simulations, shows around ~10% performance benefit:
Brief summary of changes
MuscleDynamicsInfotoFiberVelocityInfo,Muscle::calcMuscleDynamicsInfo(),Muscle::getMuscleDynamicsInfoand the cached variable.Millard2012AccelerationMusclefor caching the muscle-specific curve values.FiberVelocityInfoCachecontaining theMuscleLengthInfoandFiberVelocityInfoin a single structfiberForceLengthMultiplierandfiberActiveForceLengthMultiplierfromMuscleLengthInfotoFiberVelocityInfo(since they are used to compute forces/velocities and not lengths).calcmethods onFiberVelocityInfoCachevariable for the removed fields mentioned above.Some notes:
MuscleForceInfo, but adding that to this PR will create a very large diff, so that would be a follow up PR.Testing I've completed
Looking for feedback on
@aseth1 would be great if you have time to take a look at
Muscle.h. I moved the curve computations fromMuscleLengthInfotoFiberVelocityInfobecause they appeared a byproduct of computing the fiber force. But I might have misunderstood the invalidating behavior of CMC.@adamkewley I left the
calcmethods inMuscle.hin the header e.g.Muscle.h::685CHANGELOG.md (choose one)